fix(seedless-onboarding): update tests for optional refreshToken and proactive renewal#8148
fix(seedless-onboarding): update tests for optional refreshToken and proactive renewal#8148himanshuchawla009 wants to merge 9 commits intomainfrom
Conversation
…proactive renewal - Replace stale concurrent-rotation test with assertion that authenticate never receives refreshToken from the token-refresh path - Add tests verifying renewRefreshToken is called proactively after vault update when vault is unlocked, and skipped when locked - Add test covering the renewRefreshToken error-swallow path in refreshAuthTokens (catch block log call) - Add renewRefreshToken tests for vault-unlocked (no password) path and vault-locked-no-password error path - Fix two existing tests that captured state.refreshToken after the call; proactive renewal now rotates the token, so capture it before the call
Token expiry helpers now use a 90% lifetime threshold: - Access/metadata tokens refresh when <10% lifetime remains (uses iat) - Node auth tokens fall back to exact-expiry check (no reliable iat) Introduces module-level isTokenNearExpiry(exp, iat?) helper used by checkNodeAuthTokenExpired, checkMetadataAccessTokenExpired, and checkAccessTokenExpired.
Cover all source changes introduced in this branch: - authenticate() optional refreshToken param - proactive renewRefreshToken after JWT refresh in #doRefreshAuthTokens - renewRefreshToken vault-unlock path (optional password + skipLock) - 90% lifetime threshold for proactive token expiry checks
|
@metamaskbot publish-preview |
| vaultData: updatedVaultData, | ||
| pwEncKey, | ||
| }); | ||
|
|
There was a problem hiding this comment.
should we add option to skip proactive renewRefreshToken ?
There was a problem hiding this comment.
good suggestion, would be useful to skip in some cases.
| */ | ||
| async renewRefreshToken(password: string): Promise<void> { | ||
| return await this.#withControllerLock(async () => { | ||
| async renewRefreshToken( |
There was a problem hiding this comment.
should we consider to allow renewRefreshToken happend only when the seedless controller vault is unlocked?
we can always call submitPassword before renewRefreshToken if there are scenario where the vault is locked and we have password
There was a problem hiding this comment.
Agree with this. We can get rid of password from the params and rely on the cached encryptionKey only (which is only available when the vault is unlocked).
| // NOTE: refreshToken is intentionally omitted — renewRefreshToken is the | ||
| // sole owner of state.refreshToken. Passing it here would risk overwriting | ||
| // a token that renewRefreshToken rotated concurrently during the async | ||
| // toprfClient.authenticate() call below. |
There was a problem hiding this comment.
is it cleaner if we have re-authenticate function which only accept
idTokens,
accessToken,
metadataAccessToken,
Instead making the authenticate accept different input structure type
There was a problem hiding this comment.
Yes, I think refactor the authenticate would be better, so that we can make stricter validation, i.e. first time authentication must provide accessToken and revokeToken while re-authenticate may not.
| */ | ||
| async renewRefreshToken(password: string): Promise<void> { | ||
| return await this.#withControllerLock(async () => { | ||
| async renewRefreshToken( |
There was a problem hiding this comment.
I noticed we call this method internally, inside #doRefreshAuthTokens() so, I think we can make this private already.
Clients should not decide when to call this method, imo.
| // NOTE: refreshToken is intentionally omitted — renewRefreshToken is the | ||
| // sole owner of state.refreshToken. Passing it here would risk overwriting | ||
| // a token that renewRefreshToken rotated concurrently during the async | ||
| // toprfClient.authenticate() call below. |
There was a problem hiding this comment.
Yes, I think refactor the authenticate would be better, so that we can make stricter validation, i.e. first time authentication must provide accessToken and revokeToken while re-authenticate may not.
- Make renewRefreshToken private (#rotateRefreshToken): vault-unlock only,
no password param — callers must unlock first via submitPassword
- Add #reAuthenticate private method for JWT-refresh re-auth path, accepting
only {idTokens, accessToken, metadataAccessToken} from the refresh service
- Add skipRenewRefreshToken option to refreshAuthTokens to allow callers to
opt out of the proactive rotation
- Update tests: renewRefreshToken describe converted to #rotateRefreshToken
(tested via refreshAuthTokens), add skipRenewRefreshToken test, add
skipLock branch coverage for authenticate
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts
Show resolved
Hide resolved
packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts
Show resolved
Hide resolved
- fetchMetadataAccessCreds: use isTokenNearExpiry(exp, iat) instead of exact expiry so it is consistent with checkMetadataAccessTokenExpired - isTokenNearExpiry: guard against malformed tokens where iat >= exp (zero or negative lifetime) by falling back to exact-expiry check
packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts
Outdated
Show resolved
Hide resolved
… explicit options Callers passing explicit options (e.g. skipRenewRefreshToken: true) must not be served by an in-flight request that was started with different options. Only default-options calls (no explicit flags) share the pending promise; explicit-options calls each get their own independent request.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts
Show resolved
Hide resolved
| * | ||
| * @returns A Promise that resolves to void. | ||
| */ | ||
| async #rotateRefreshToken(): Promise<void> { |
There was a problem hiding this comment.
Just to confirm the private behaviour
with this private, the parent cannot rotateRefreshToken (renewRefreshToken), it can now only revokePendingToken.
…ents and logging - Remove redundant `skipRenewRefreshToken` option from `refreshAuthTokens` since the `#cachedDecryptedVaultData` guard already prevents rotation when the vault is locked - Use project `log()` instead of `console.error` for rotation failure - Fix stale JSDoc on `authenticate` that referenced removed internal usage - Update `checkMetadataAccessTokenExpired` and `checkAccessTokenExpired` JSDoc to mention the 90% proactive refresh threshold - Add `VaultLocked` error constant with defensive guard in `#rotateRefreshToken` - Remove related tests for the deleted option
3 participants
Explanation
References
Checklist
Note
Medium Risk
Changes token refresh/re-auth flows and refresh/revoke token rotation, including a breaking removal of the public
renewRefreshTokenAPI; mistakes could cause unexpected logouts or failed refreshes. The scope is contained to the seedless onboarding controller with extensive test updates.Overview
Tightens the token-refresh path by introducing a private
#reAuthenticatethat re-authenticates using only JWT-refresh outputs (idTokens,accessToken,metadataAccessToken), rather than reusing the broader publicauthenticateinputs.After a successful JWT refresh, the controller now proactively rotates the refresh/revoke token pair via private
#rotateRefreshTokenwhen the vault is unlocked (persisting the newrevokeTokento the vault first, then updating state and queuing the old pair for revocation); rotation failures are logged but do not failrefreshAuthTokens.Token staleness checks for
accessToken/metadataAccessTokennow use a 90% lifetime (needsiat) proactive-refresh threshold, while node auth tokens fall back to exact-expiry checking. The PR also addsVaultLockederror messaging, updates the changelog (including breaking removal of publicrenewRefreshToken), and expands/adjusts tests to cover the new refresh, rotation, and expiry behaviors.Written by Cursor Bugbot for commit d3b0814. This will update automatically on new commits. Configure here.